-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Week4 필수과제 구현 #8
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 처음할때 서버통신이 정말 어려웠는데 에러코드 별로 분기처리도 잘하시고
DataSource로 토큰도 잘 가지고 계시네요 ㄷㄷ
고생하셨습니다!! 합세 빠이팅~
(여행갔다와서 리뷰 마저 더 달수도 ㅋㅋ)
response.code() == 400 -> { | ||
val errorMessage = when(response.body()?.code) { | ||
"00" -> "요청이 유효하지 않습니다" | ||
"01" -> "입력값이 8자를 초과했습니다" | ||
else -> "회원가입에 실패했습니다" | ||
} | ||
_uiState.update { it.copy(errorMessage = errorMessage) } | ||
} | ||
response.code() == 409 -> { | ||
_uiState.update { it.copy(errorMessage = "이미 존재하는 username입니다") } | ||
} | ||
else -> { | ||
_uiState.update { it.copy(errorMessage = "회원가입에 실패했습니다") } | ||
} | ||
} | ||
true | ||
} catch (e: Exception) { | ||
_uiState.update { it.copy(errorMessage = "네트워크 오류가 발생했습니다") } | ||
} finally { | ||
_uiState.update { it.copy(isLoading = false) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이야 잘해놓으셨네요!!
val route: Any | ||
val route: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
난 아직도 왜 해결된건지 이유를 모름 ㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진짜 모름...왜됨..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
무슨 문제가 있었는데 !!
data class BaseResponse<T>( | ||
@SerialName("result") | ||
val result: T? = null, | ||
@SerialName("code") | ||
val code: String? = null | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseResponse 쓰는거 좋은데요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
명세서에 겹치는 부분은 이렇게 만드는게 좋다고 하더라구요! 그래서 적용해봄쓰
class AuthLocalDataSource(private val context: Context) { | ||
private val tokenKey = stringPreferencesKey("token") | ||
|
||
suspend fun saveToken(token: String) { | ||
context.dataStore.edit { preferences -> | ||
preferences[tokenKey] = token | ||
} | ||
} | ||
|
||
fun getToken(): Flow<String?> = context.dataStore.data.map { preferences -> | ||
preferences[tokenKey] | ||
} | ||
|
||
suspend fun clearToken() { | ||
context.dataStore.edit { preferences -> | ||
preferences.remove(tokenKey) | ||
} | ||
} | ||
|
||
companion object { | ||
@Volatile | ||
private var instance: AuthLocalDataSource? = null | ||
|
||
fun getInstance(context: Context): AuthLocalDataSource { | ||
return instance ?: synchronized(this) { | ||
instance ?: AuthLocalDataSource(context.applicationContext).also { | ||
instance = it | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실제 구현부분은 AuthLocalDataSourceImpl 라고 따로 구현하는것도 좋습니다!!
나중에 모듈로 binding해주거나
val context = navController.context val userInfoLocalDataSource = UserInfoLocalDataSourceImpl(context)
이렇게 선언한걸 뷰모델에 넣어서 사용하기도 합니다
제 코드에 힐트 적용하고나서 예시로 보여드릴게요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네! 아직 의존성 주입이 왜 필요한지 왜 쓰이는지 겪어보지 않아서 덜 와닿는거 같아요. DI더 공부해보고 저도 물어보겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 :)
이번주부터 합세 시작이라고 들었는데 화이팅이에요!
@@ -17,26 +16,21 @@ import org.sopt.and.presentation.navigation.NavGraph | |||
@Composable | |||
fun MainScreen() { | |||
val navController = rememberNavController() | |||
var bottomNaviVisible by remember { mutableStateOf(false) } | |||
var userEmail by remember { mutableStateOf("") } | |||
var isLoggedIn by remember { mutableStateOf(false) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수명에서 조금 더 고민을 해보면 좋을 것 같습니다.
현재는 로그인을 한다면 바텀바가 보이고 있지만, 추후에는 조건이 달라질 수 있어요.
그렇기 때문에 isLoggedIn
과 같이 조건이 적혀있는 변수명을 사용한다면 추후 조건이 변경되었을 때 변수명도 변경해야 하는 경우가 생깁니다
data class MyPageUiState( | ||
val hobby: String = "", | ||
val isLoading: Boolean = false, | ||
val errorMessage: String? = null | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 class가 ViewModel 내부에 존재하는 이유가 있을까요?
init { | ||
fetchMyHobby() | ||
} | ||
|
||
fun fetchMyHobby() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰모델 내부에서만 호출한다면 private 접근제한자를 붙이면 더욱 안전하게 사용할 수 있어요
} catch (e: Exception) { | ||
_uiState.update { | ||
it.copy( | ||
errorMessage = "네트워크 오류가 발생했습니다", | ||
isLoading = false | ||
) | ||
} | ||
} | ||
} else { | ||
_uiState.update { | ||
it.copy( | ||
errorMessage = "로그인이 필요합니다", | ||
isLoading = false | ||
) | ||
} | ||
} | ||
} | ||
} catch (e: Exception) { | ||
_uiState.update { | ||
it.copy( | ||
errorMessage = "토큰 조회에 실패했습니다", | ||
isLoading = false | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch를 할 때에는 Exception의 종류에 대해서 명확하게 작성해주시는게 좋습니다. 그래야 추후 오류를 파악하고 수정하기에 좋아요
그리고 모든 부분의 마지막에 isLoading = false
라는 코드를 작성해주고 있는데 이는 finally로 빼는건 어떨까요? 중복 코드를 줄일 수 있을 것 같아요.
isError = uiState.errorMessage?.contains("이메일") == true | ||
TextInputField( | ||
value = uiState.username, | ||
onValueChange = { signInViewModel.onUsernameChange(it) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onValueChange = { signInViewModel.onUsernameChange(it) }, | |
onValueChange = signInViewModel::onUsernameChange, |
이런 방법으로도 작성할 수 있어요. 컴포즈에서는 이러한 방법이 확실한 장점이 있는 반면, 단점도 있어요. 한번 찾아보시고 상황에 맞게 사용하시면 좋을 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthRequest 파일 안에 SignUpRequest, SignInRequest 을 한번데 넣어서 관리하시는군요..! 나중에 유저 인증 관련 기능이 더 복잡해질 걸 생각하면, 이런 관리 방식은 굉장히 효율적인 것 같아요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthResponse 안에 응답 관련 클래스도 다 넣어두니 훨씬 깔끔하네요!!
if (token != null) { | ||
try { | ||
val response = ServicePool.authService.getMyHobby(token) | ||
when { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 분기 처리가 엄청 자세해서 거의 모범 코드 같아요... 보고 많이 배워갑니다!! 이번 과제 수정할 때 참고해볼게용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
늘 열심히 하시는 팀원이 같은 조에 계셔서 든든합니다👍 서버통신까지 높은 수준으로 완성해 보고자 노력하신 게 느껴져요!! 고생하셨습니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 ~ 합동 세미나 파이팅
implementation("androidx.datastore:datastore-preferences:1.0.0") | ||
implementation("androidx.datastore:datastore-preferences-core:1.0.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 버전 카탈로그 적용해주시면 좋을 것 같네요!
preferences[tokenKey] | ||
} | ||
|
||
suspend fun clearToken() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반 함수와 suspend 함수의 차이는 무엇일까요?
authLocalDataSource = AuthLocalDataSource.getInstance(LocalContext.current) | ||
) | ||
), | ||
modifier: Modifier = Modifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifer는 최상단에 올려줍시다.
순서 : 필수 인자 -> Modifier (선택 인자 중에서 가장 처음이어야 함) -> 선택 인자
init { | ||
fetchMyHobby() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init 블럭은 어떤 식으로 동작할까요?
어떤 로직을 담으면 좋고, 어떤 로직을 담지 않아야 할까요?
val route: Any | ||
val route: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
무슨 문제가 있었는데 !!
"01" -> "요청이 유효하지 않습니다" | ||
"02" -> "로그인 정보가 올바르지 않습니다" | ||
else -> "로그인에 실패했습니다" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 친구들도 상수화 해주시면 좋을 것 같네요!
Related issue 🛠
Work Description ✏️
Screenshot 📸
4.mov
4.2.mp4
Uncompleted Tasks 😅
취미 조회 응답 잘 오는데 UI 반영이 안됨
List 변경하기
To Reviewers 📢
이번에 API 적용하면서 아직까지도 파일 구조 분리에 대해 이해가 가지 않는 부분이 많아서 미흡한것 같습니다.
뭔가 규모가 큰 개발을 진행하거나 앞으로 개발을 해가면서 로직 분리의 필요성을 느낄 타이밍이 오면 그때서야 제대로 이해 할 것 같아요ㅠ